Skip to content

module: improve error message for invalid data URL#37701

Merged
aduh95 merged 1 commit intonodejs:masterfrom
aduh95:invalid-mime-data-url
Apr 3, 2021
Merged

module: improve error message for invalid data URL#37701
aduh95 merged 1 commit intonodejs:masterfrom
aduh95:invalid-mime-data-url

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 10, 2021 •
edited
Loading

Fixes: #37647

Changes the error message when importing module with an unknown/unsupported MIME type from:

TypeError [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected string to be returned for the "format" from the "loader getFormat" function but got type object.

to

TypeError [ERR_INVALID_MODULE_SPECIFIER]: Invalid module "data:application/json,[]" has an unsupported MIME type "application/json"

@aduh95 aduh95 force-pushed the invalid-mime-data-url branch from 114e4c8 to 68fcf83 Compare March 10, 2021 22:24
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 10, 2021
@targos
Copy link
Member

targos commented Mar 12, 2021

@nodejs/modules

@targos targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 12, 2021
DerekNonGeneric
DerekNonGeneric reviewed Mar 12, 2021
View reviewed changes
DerekNonGeneric
DerekNonGeneric reviewed Mar 12, 2021
View reviewed changes
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my 2¢. Not sure that error code has been used for this purpose historically.

@bmeck
Copy link
Member

bmeck commented Mar 12, 2021

CC: @ljharb since this mentions MIME to user

@bmeck
Copy link
Member

bmeck commented Mar 12, 2021

to clarify a bit, these should have been picked up and could have been generated in the past

@ljharb
Copy link
Member

ljharb commented Mar 12, 2021

Seems fine to me, since anyone using a data URL is already familiar with MIMEs (this is ofc not true for almost every other specifier category).

@aduh95 aduh95 added the review wanted PRs that need reviews. label Mar 21, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 2, 2021

Ping @nodejs/modules for reviews.

DerekNonGeneric
DerekNonGeneric approved these changes Apr 2, 2021
View reviewed changes
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from my outstanding comments (non-blocking), everything else looks pretty good to me.

I will be digging around this part of codebase in a few days, so I will report back if I find issues.

guybedford
guybedford approved these changes Apr 2, 2021
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps Unsupported MIME "text/plain" loading "data:text/plain,export default 0".

Also it could be nice if we could support the parent context for tracing where it comes from eg if it were import('data:...') in a module, it helps to know where the import expression is.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 2, 2021

@guybedford Agreed, although I think this would deserve its own PR to align with other ERR_INVALID_MODULE_SPECIFIER uses in core.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 2, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/37108/

@guybedford
Copy link
Contributor

guybedford commented Apr 2, 2021

@aduh95 I do think it's important to move the "unsupported MIME" part to the beginning of the message though. This is because the data URL could be of any length, so that when scanning the error the user should be able to easily see the "unsupported MIME" part first. It also fixes that the current error isn't quite gramatically complete without a comma or colon or something type of break otherwise.

Alternatively to make it gramatically correct:

  • Use a full stop and a new sentence - The MIME type "..." is incorrect.
  • Use a comma / colon / dash.
  • Use "has an unsupported MIME type "...""

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 2, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/37109/

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 2, 2021

I went to the last suggestion as it was the easiest to implement. I think you have a point regarding very long specifiers occulting the reason, maybe we can fix that in a similar way as proposed in #37852 (comment)?

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Apr 2, 2021

[…] maybe we can fix that in a similar way as proposed in #37852 (comment)?

I believe the feature request outlined in #37581 is precisely the error property we need.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 2, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/37110/

@aduh95
module: improve error message for invalid data URL
46062a0 
Fixes: nodejs#37647

PR-URL: nodejs#37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@aduh95 aduh95 force-pushed the invalid-mime-data-url branch from f5ed39f to 46062a0 Compare April 3, 2021 18:06
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 3, 2021

Landed in 46062a0

@aduh95 aduh95 merged commit 46062a0 into nodejs:master Apr 3, 2021
@aduh95 aduh95 deleted the invalid-mime-data-url branch April 3, 2021 18:06
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
@aduh95 @MylesBorins
module: improve error message for invalid data URL
618da81 
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2021
Merged
MylesBorins pushed a commit that referenced this pull request Apr 5, 2021
@aduh95 @MylesBorins
module: improve error message for invalid data URL
3ab9619 
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
@aduh95 @targos
module: improve error message for invalid data URL
079671d 
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybedford guybedford guybedford approved these changes

+1 more reviewer

@DerekNonGeneric DerekNonGeneric DerekNonGeneric approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. question Issues that look for answers. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Import from JSON data URL doesn't work

7 participants

@aduh95 @targos @bmeck @ljharb @nodejs-github-bot @guybedford @DerekNonGeneric